Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix W.splitFileName "/\\?/a:" #220

Merged
merged 5 commits into from
Jan 26, 2024
Merged

Fix W.splitFileName "/\\?/a:" #220

merged 5 commits into from
Jan 26, 2024
+271 −54

Conversation

Bodigrim
Copy link
Contributor

Fixes #219.

hasufell
hasufell reviewed Jan 23, 2024
View reviewed changes
System/FilePath/Internal.hs
, null bs''
, Just (drive, rest) <- readDriveLetter file
-> (dirSlash <> drive, rest)
_ -> (dirSlash, file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line is redundant? I'tll fall through to | otherwise, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it does not fall through, the line is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I misread the code.

@hasufell
Copy link
Member

hasufell commented Jan 23, 2024 •
edited
Loading

We have equivalence tests that compare this function with the old implementation. It's depressing that it wasn't caught. The quickcheck generators are not good enough and don't produce enough "odd data".

I think one way forward is to implement the ABNF that I have here: https://github.com/hasufell/filepath/blob/16e5374620189b27eca1eed09642ec02b2222fc8/System/FilePath/Internal/Parser.hs#L157-L165

...at least for the test suite and use that to generate windows filepaths.

hasufell
hasufell reviewed Jan 23, 2024
View reviewed changes
System/FilePath/Internal.hs
-- Otherwise, since s1 is a path separator, we might be in the middle of UNC path.
, null bs' || maybe False isIncompleteUNC (readDriveUNC dirSlash)
-> (fp, mempty)
-- This handles inputs like "//?/A:" and "//?/A:foo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivia: //?/A:foo is actually invalid in some sense... it's hard to interpret it. UNC paths are absolute, but A:foo on its own is a relative directory (relative to current working dir on drive A). The windows API probably won't care, but no such file can ever exist, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but filepath-1.4.2.2 does not mind to process //?/A:foo even if it's complete nonsense.

@Bodigrim
Copy link
Contributor Author

I started this PR with thinking about properties of splitFileName. One of them is that takeDrive x isPrefixOf dropFileName x. Testing such property indeed reveals the originally reported issue with \\?\a: and also \\?\a:foo.

@hasufell
Copy link
Member

hasufell commented Jan 24, 2024 •
edited
Loading

I pushed updated equivalence tests that now make use of the ABNF to generate more odd cases. Lots of new failures:

$ cabal run --enable-tests filepath-equivalent-tests -- --quickcheck-tests 50_000 -p 'windows'
Running 1 test suites...
Test suite filepath-equivalent-tests: RUNNING...
equivalence
  pathSeparator (windows):             OK
    +++ OK, passed 1 test.
  pathSeparators (windows):            OK
    +++ OK, passed 1 test.
  isPathSeparator (windows):           OK (0.13s)
    +++ OK, passed 50000 tests.
  searchPathSeparator (windows):       OK
    +++ OK, passed 1 test.
  isSearchPathSeparator (windows):     OK (0.13s)
    +++ OK, passed 50000 tests.
  extSeparator (windows):              OK
    +++ OK, passed 1 test.
  isExtSeparator (windows):            OK (0.17s)
    +++ OK, passed 50000 tests.
  splitSearchPath (windows):           OK (2.90s)
    +++ OK, passed 50000 tests.
  splitExtension (windows):            OK (0.60s)
    +++ OK, passed 50000 tests.
  takeExtension (windows):             OK (0.74s)
    +++ OK, passed 50000 tests.
  replaceExtension (windows):          OK (0.99s)
    +++ OK, passed 50000 tests.
  dropExtension (windows):             OK (0.62s)
    +++ OK, passed 50000 tests.
  addExtension (windows):              OK (0.94s)
    +++ OK, passed 50000 tests.
  hasExtension (windows):              FAIL (0.03s)
    *** Failed! Falsified (after 3395 tests and 1 shrink):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[UnixSep,UnixSep,UnixSep,WindowsSep,UnixSep,UnixSep,UnixSep,WindowsSep,WindowsSep] (Rel2 FileName (NonEmptyString ','":|""9\157814") (Just DS1 (NonEmptyString '('":|"".R") Nothing))) (\\?\a:////\///\\,9𦡶:(.R)
    Use --quickcheck-replay=129836 to reproduce.
    Use -p '/windows/&&/hasExtension (windows)/' to rerun this test only.
  splitExtensions (windows):           FAIL (0.12s)
    *** Failed! Falsified (after 8118 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[UnixSep,UnixSep,WindowsSep,WindowsSep,WindowsSep,WindowsSep,UnixSep,UnixSep,WindowsSep] (Rel2 FileName (NonEmptyString '<'":|""\SUB\36214\a\SIo\f\EOT?\SYN\40354g.\t\16570:") Nothing)) (\\?\a:///\\\\//\<赶o
                                                                                                        ?鶢g.	䂺:)
    Use --quickcheck-replay=619351 to reproduce.
    Use -p '/windows/&&/splitExtensions (windows)/' to rerun this test only.
  dropExtensions (windows):            FAIL (0.23s)
    *** Failed! Falsified (after 12623 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[WindowsSep] (Rel2 FileName (NonEmptyString '\154525'":|"".") Nothing)) (\\?\a:\\𥮝.)
    Use --quickcheck-replay=39137 to reproduce.
    Use -p '/windows/&&/dropExtensions (windows)/' to rerun this test only.
  takeExtensions (windows):            OK (0.40s)
    +++ OK, passed 50000 tests.
  replaceExtensions (windows):         FAIL (0.19s)
    *** Failed! Falsified (after 8184 tests and 3 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[UnixSep] (Rel2 FileName (NonEmptyString '\11591'":|"".r") Nothing)) (\\?\a:\/ⵇ.r)
    ""
    Use --quickcheck-replay=673584 to reproduce.
    Use -p '/windows/&&/replaceExtensions (windows)/' to rerun this test only.
  isExtensionOf (windows):             OK (0.51s)
    +++ OK, passed 50000 tests.
  stripExtension (windows):            OK (0.45s)
    +++ OK, passed 50000 tests.
  splitFileName (windows):             FAIL
    *** Failed! Falsified (after 245 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,UnixSep] (Rel2 FileName (NonEmptyString '('":|""") Nothing)) (\\?\a:/\/()
    Use --quickcheck-replay=177366 to reproduce.
    Use -p '/windows/&&/splitFileName (windows)/' to rerun this test only.
  takeFileName (windows):              FAIL
    *** Failed! Falsified (after 157 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,WindowsSep,WindowsSep,WindowsSep,UnixSep] (Rel2 FileName (NonEmptyString '\1096994'":|""wUN\97297if") Nothing)) (\\?\a:/\\\\/􋴢wUN𗰑if)
    Use --quickcheck-replay=369219 to reproduce.
    Use -p '/windows/&&/takeFileName (windows)/' to rerun this test only.
  replaceFileName (windows):           FAIL
    *** Failed! Falsified (after 414 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[WindowsSep,WindowsSep,WindowsSep,WindowsSep,WindowsSep,UnixSep,WindowsSep,WindowsSep,UnixSep,UnixSep] (Rel2 FileName (NonEmptyString 'v'":|""\RS}\128611(4") Nothing)) (\\?\a:\\\\\\/\\//v}🙣(4)
    ""
    Use --quickcheck-replay=357124 to reproduce.
    Use -p '/windows/&&/replaceFileName (windows)/' to rerun this test only.
  dropFileName (windows):              FAIL
    *** Failed! Falsified (after 231 tests and 1 shrink):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[UnixSep,UnixSep,UnixSep,UnixSep,WindowsSep,UnixSep] (Rel2 FileName (NonEmptyString '.'":|""'\SYNL\83354\ACK\SO_") Nothing)) (\\?\a://///\/.'L𔖚_)
    Use --quickcheck-replay=328899 to reproduce.
    Use -p '/windows/&&/dropFileName (windows)/' to rerun this test only.
  takeBaseName (windows):              FAIL
    *** Failed! Falsified (after 740 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[WindowsSep,UnixSep,UnixSep,WindowsSep,WindowsSep,UnixSep,WindowsSep,UnixSep,WindowsSep,WindowsSep,UnixSep,UnixSep,WindowsSep] (Rel2 FileName (NonEmptyString ')'":|""w\1028602\143532G<DQ~%\1043888V8\DLE\993471\188230Y") Nothing)) (\\?\a:\\//\\/\/\\//\)w󻇺𣂬G<DQ~%󾶰V8󲢿𭽆Y)
    Use --quickcheck-replay=581857 to reproduce.
    Use -p '/windows/&&/takeBaseName (windows)/' to rerun this test only.
  replaceBaseName (windows):           FAIL
    *** Failed! Falsified (after 379 tests and 3 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep] (Rel2 FileName (NonEmptyString '\NUL'":|""\1056142GWL|") Nothing)) (\\?\a:/\􁶎GWL|)
    ""
    Use --quickcheck-replay=609582 to reproduce.
    Use -p '/windows/&&/replaceBaseName (windows)/' to rerun this test only.
  takeDirectory (windows):             FAIL
    *** Failed! Falsified (after 993 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,WindowsSep,WindowsSep,UnixSep,UnixSep,WindowsSep] (Rel2 FileName (NonEmptyString '\1085460'":|""\DC1XJg%d\1074770J8@A") Nothing)) (\\?\a:/\\\//\􉀔XJg%d􆙒J8@A)
    Use --quickcheck-replay=443621 to reproduce.
    Use -p '/windows/&&/takeDirectory (windows)/' to rerun this test only.
  replaceDirectory (windows):          FAIL
    *** Failed! Falsified (after 188 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,WindowsSep,WindowsSep,UnixSep,UnixSep] (Rel2 FileName (NonEmptyString '\62007'":|""\DC1F") Nothing)) (\\?\a:/\\\//F)
    ""
    Use --quickcheck-replay=875347 to reproduce.
    Use -p '/windows/&&/replaceDirectory (windows)/' to rerun this test only.
  combine (windows):                   OK (0.81s)
    +++ OK, passed 50000 tests.
  splitPath (windows):                 OK (0.75s)
    +++ OK, passed 50000 tests.
  joinPath (windows):                  OK (0.90s)
    +++ OK, passed 50000 tests.
  splitDirectories (windows):          OK (0.89s)
    +++ OK, passed 50000 tests.
  splitDrive (windows):                OK (0.54s)
    +++ OK, passed 50000 tests.
  joinDrive (windows):                 OK (0.82s)
    +++ OK, passed 50000 tests.
  takeDrive (windows):                 OK (0.23s)
    +++ OK, passed 50000 tests.
  hasDrive (windows):                  OK (0.18s)
    +++ OK, passed 50000 tests.
  dropDrive (windows):                 OK (0.52s)
    +++ OK, passed 50000 tests.
  isDrive (windows):                   OK (0.23s)
    +++ OK, passed 50000 tests.
  hasTrailingPathSeparator (windows):  OK (0.30s)
    +++ OK, passed 50000 tests.
  addTrailingPathSeparator (windows):  OK (0.53s)
    +++ OK, passed 50000 tests.
  dropTrailingPathSeparator (windows): OK (0.53s)
    +++ OK, passed 50000 tests.
  normalise (windows):                 OK (1.14s)
    +++ OK, passed 50000 tests.
  equalFilePath (windows):             OK (1.43s)
    +++ OK, passed 50000 tests.
  makeRelative (windows):              OK (2.17s)
    +++ OK, passed 50000 tests.
  isRelative (windows):                OK (0.19s)
    +++ OK, passed 50000 tests.
  isAbsolute (windows):                OK (0.19s)
    +++ OK, passed 50000 tests.
  isValid (windows):                   OK (0.41s)
    +++ OK, passed 50000 tests.
  makeValid (windows):                 OK (1.03s)
    +++ OK, passed 50000 tests.

12 out of 48 tests failed (23.10s)

@hasufell
Copy link
Member

More specifically for splitFileName:

This PR:

ghci> System.FilePath.Windows.splitFileName "\\\\?\\a:///\\\\\\\\k1"
("\\\\?\\a:///\\\\\\\\k1","")

filepath-1.4.2.2:

ghci> System.FilePath.Windows.splitFileName "\\\\?\\a:///\\\\\\\\k1"
("\\\\?\\a:///\\\\\\\\","k1")

@hasufell
Copy link
Member

The bug was in hasPenultimateColon

hasufell
hasufell reviewed Jan 25, 2024
View reviewed changes
System/FilePath/Internal.hs
| isPathSeparator s1
, isPathSeparator s2
, Just (s3, s4, bs'') <- uncons2 bs'
, s3 == _question
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is in fact a bug (although the old filepath behaves the same way):

ghci> System.FilePath.Windows.splitFileName "\\\\.\\a:foo"
("\\\\.\\","a:foo")

There are three UNC namespaces:

  • file namespace: //?/
  • device namespace: //./
  • NT namespace /??/

I'm still unsure whether fixing those bugs will cause more good than harm, but those are bugs.

@hasufell hasufell merged commit 74713b9 into master Jan 26, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasufell hasufell hasufell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

possible reversion of W.splitFileName "/\\?/a:"
2 participants
@Bodigrim @hasufell